Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove waitForSelector for some dashboards page #10

Merged
merged 7 commits into from
Jan 21, 2023
Merged

Remove waitForSelector for some dashboards page #10

merged 7 commits into from
Jan 21, 2023

Conversation

rupal-bq
Copy link
Member

Description

  • Removed extra handling for some pages to support all dahsboards pages
  • Added a note that tenant in url has highest priority

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
@rupal-bq
Copy link
Member Author

Signed-off-by: Rupal Mahajan <maharup@amazon.com>
@rupal-bq rupal-bq marked this pull request as ready for review January 20, 2023 21:54
@rupal-bq rupal-bq requested a review from a team as a code owner January 20, 2023 21:54
const reportSource = getReportSourceFromURL(url);

// if its an OpenSearch report, remove extra elements.
if (reportSource !== 'Other' && reportSource !== 'Saved search') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember removing extra buttons was implemented to reduce unnecessary information in the report, is it ok to delete this?

If other page url (e.g. alerting) is used, reportSource would be Other so this logic and waitForSelector should be skipped. This should not make a difference?

Copy link
Member Author

@rupal-bq rupal-bq Jan 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For alerinng it will work, but I tried anomaly detection, url: <dashboard_endpoint>/_dashboards/app/anomaly-detection-dashboards#/
Since url had dashboards#' cli got stuck waiting for #dashboardViewport

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can keep this if we have some identifiers which can't be part of other opensearch url's. I also compared the pdf report with and without this change. There's not much visible differences so I think it's ok to remove this for now. Do you have any other suggestion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For alerinng it will work, but I tried anomaly detection, url: <dashboard_endpoint>/_dashboards/app/anomaly-detection-dashboards#/ Since url had dashboards#' cli got stuck waiting for #dashboardViewport

You can make it matching more specific, maybe /app/dashboards#. It's ok if not much difference, did you try visualize though? I remember it removes the vis editor

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier I only tried dashboard, just tried visualize. I didn't notice much difference.
With main branch: opensearch-report-2023-01-20T22:41:33.335Z-main.pdf
With this change: opensearch-report-2023-01-20T22:39:37.417Z-fix.pdf

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change the constant to

export const URL_SOURCE = {
  DASHBOARDS: "/app/dashboards#",
  VISUALIZE: "/app/visualize#",
  DISCOVER: "/app/discover#",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this change and removed waitForSelector.

Signed-off-by: Rupal Mahajan <maharup@amazon.com>

This reverts commit a18a67e.
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
Signed-off-by: Rupal Mahajan <maharup@amazon.com>

This reverts commit c1834f6.
@rupal-bq rupal-bq changed the title Remove extra handling for some dashboards page Remove waitForSelector for some dashboards page Jan 20, 2023
@rupal-bq rupal-bq merged commit 4cf7732 into opensearch-project:main Jan 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants